-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enhance - all machine learning resource - bump API to v2022-05-01
#18279
Conversation
* update testcase
application_insights_id = azurerm_application_insights.test.id | ||
key_vault_id = azurerm_key_vault.test.id | ||
storage_account_id = azurerm_storage_account.test.id | ||
public_network_access_enabled = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type should be enum, not boolean, it's not like binary.
It could be many possible values, such as "Enabled", "Disabled", "EnabledForSpecificNetwork", etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally if the enum value only have enable/disable
, we will convert it to boolean, if the possible values more then two, I will change it back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currently afaik it only supports two values in nearly every resource. if another value is ever add we can always deprecate the property and add non boolean property
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please keep this to an enum type for consistency with the Azure APIs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@katbyte @deeikele and @mingweihe are from MLW service team, they suggest we should keep the enum type consistecy with the AzureAPI and a potential type EnabledForSpecificNetwork
will be added in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@katbyte @deeikele and @mingweihe are from MLW service team, they suggest we should keep the enum type consistecy with the AzureAPI and a potential type
EnabledForSpecificNetwork
will be added in the future.
Exactly, I also synced up with the team, they mentioned even though the swagger's having two values for the properties, the new value will be added later.
I can understand terraform has its own code change policy, while this is really a particular case, keeping a mismatched type there is not suitable for future maintenance.
Meanwhile, I think it's good to follow Azure APIs mostly, enum type with two values auto-changing to boolean policy is interesting, but not that suitable here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To amplify Mingwei's response boolean type is not appropriate here. Reference to enum implementation is found in Iot Hub (https://learn.microsoft.com/en-us/azure/iot-hub/iot-hub-public-network-access) selected IP ranges
accepted value.
internal/services/machinelearning/machine_learning_workspace_resource.go
Outdated
Show resolved
Hide resolved
internal/services/machinelearning/machine_learning_workspace_resource.go
Outdated
Show resolved
Hide resolved
internal/services/machinelearning/machine_learning_workspace_resource.go
Outdated
Show resolved
Hide resolved
* public_access_behind_virtual_network_enabled deprecate switch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as discussed internally LGTM ⚙️
This functionality has been released in v3.25.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you! |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Fixes: #16177 #16836 #17170
API issue: Azure/azure-rest-api-specs#18601
2021-07-01
to2022-05-01
.azurerm_machine_learning_workspace
support new config:public_network_access_enabled
andv1_legacy_mode
public_access_behind_virtual_network_enabled
inazurerm_machine_learning_workspace
- renamepublic_network_access_enabled
#16288 and mark this property to be deprecate in v4 . There is a API breaking change afterv2021-10-01
(@deeikele to clarify if I'm wrong),allowPublicAccessWhenBehindVnet
will be replaced bypublicNetworkAccess
. In this PRpublic_access_behind_virtual_network_enabled
will be deprecated and replaced bypublic_network_access_enabled
, also I removed the version judugment and putpublic_network_access_enabled
in the general config.